Periodic VICE notifications #10
Conversation
analyses.go
Outdated
| LEFT join notif_statuses ON jobs.id = notif_statuses.analysis_id | ||
| WHERE jobs.status = $1 | ||
| AND (notif_statuses.last_periodic_warning is null | ||
| OR notif_statuses.last_periodic_warning < $2 - (coalesce(notif_statuses.periodic_warning_period, 14400)::text || ' seconds'::text)::interval) |
There was a problem hiding this comment.
This query looks like it should work to me, but I found myself wondering whether it would be reasonable to make a couple small changes. The first was to replace the argument with a call to Postgres's CURRENT_TIMESTAMP function. The other was to change the type of notif_statuses.periodic_warning_period to an interval just to eliminate the need to do a string concatenation in order for the type conversion to work. There might be some implications that I hadn't thought of, but I was thinking it might simplify the query a little at the cost of making insertions a little more verbose.
There was a problem hiding this comment.
On those two thoughts:
a.) I didn't use now or CURRENT_TIMESTAMP (equivalent) mostly because I wanted to be able to use the same timestamp for everything, but looking at it, this function is using its own call to time.Now and then the loop in main.go is the only one actually reusing it. So I think I agree and we can switch this to that, just to remove another place something can go wrong.
b.) I like this, though I think it basically just means we use string concatenation on the insert end instead. Still, that should mean the query that's called much more often (this one) is better.
There was a problem hiding this comment.
I put up cyverse-de/de-database#26 to change that type. I'll look at the updates that need to happen here and try to get those up in this PR concurrently.
There was a problem hiding this comment.
Sounds good. Yeah, unfortunately, I don't see a good way to eliminate the string concatenation from both SQL statements either.
| const PeriodicMessageFormat = `Analysis "%s" (%s) is still running. | ||
|
|
||
| This is a regularly scheduled reminder message to ensure you don't use up your quota. | ||
|
|
||
| This job has been running since %s (%s).` | ||
|
|
||
| // PeriodicSubjectFormat is the parameterized subject for the email that is sent | ||
| // to users as a regular reminder of a running job | ||
| // parameters: analysis name, start date, duration | ||
| const PeriodicSubjectFormat = `Analysis %s is running since %s (%s)` |
There was a problem hiding this comment.
I received a request to have this message sent as an HTML message to make it more user friendly. That doesn't have to be done as part of this phase, but eventually, we'll probably want to use a template in de-mailer to format a user friendly message.
There was a problem hiding this comment.
That's a good point. I think we'll need to modify some of the notification stuff to allow passing in an email template, but that's certainly better. I might merge this to get it into QA for testing soonish (ideally, before I'm out for a few days coming up here), but I'll try to get something minimal into de-mailer soon. If there's a template already being used specifically for this/in place it should be easier to update the content to meet requirements anyway.
|
I've updated this code to use a postgresql interval, hopefully correctly. I'm going to go ahead and get it in, but further reviews or re-reviews are of course still welcome. I'm using https://pkg.go.dev/github.com/sanyokbig/pqinterval which I haven't before, but it looks like it should work for having a properly scannable postgresql interval. |
Okay, I think this is ready to get some eyes on it. How this should work is:
Right now this is using a default value for the warning period (14400s, so 4 hours). I think that should come from preferences, but I'd like to get more of this wired up and then come back to that, if I can.
As far as review, I'm worried I've got an inequality backwards somewhere or that I'm going to end up spamming the database with queries/bog things down in general. But all review is welcome, I'm not super familiar with timelord even now.